Fix field lookup in cyclic structs #184
Conversation
|
Still evaluating, so marking as draft for the time being. |
|
Evaluation (internal link) was unremarkable. @sauyon, if you can find the time it would be great if you could take a look at this over the weekend so we can get it into next week's dist upgrade. |
There was a problem hiding this comment.
Sorry, it took me some time to work out exactly what was going wrong.
Having not looked at the nomad code at all, I'm actually reasonably surprised that people would want cyclic embedded types at all; that seems like a recipe for confusion. Perhaps it's a type system hack of some kind.
I think you're right that this should fix the issue, since if there is a cycle the recursion must eventually hit a type that has the field in question, which should then stop the recursion.
I assume if the field does not actually exist, no tuples are generated and so the fixed point converges after one step? (According to my somewhat shaky understanding of the semantics of recursion)
Yeah, I think you're right. It doesn't sound terribly useful.
Kind of. Fixpoints are computed iteratively and bottom-up, so in the first iteration we'll get all fields declared directly in the type, with depth |
|
The cyclic type is Server: its embedded fields rpcHandler, planner, and nodeHeartbeater each in turn embed |
github#184 added a regression test for the non-termination it was fixing. The fix hasn't made it into Code Scanning yet, so for the time being it will fail with precisely that non-termination when analysing the regression tests.
|
hey folks 👋 I was wondering if I could confirm that this got included in with the most recent codeql bundle? |
|
I can confirm that it is deployed on LGTM.com. @robertbrignull, I assume this means it's also part of the most recent bundle? |
|
The codeql-go commit currently used on code scanning is |
Our analysis wasn't terminating on https://github.com/hashicorp/nomad due to it trying to look up a field in a cyclic struct. (I'm quite surprised this hasn't happened earlier.)
The problem is in the part of our library that attempts to model the rules for resolving field or method references
x.fin the presence of embedded fields. While you might think that it is an error to embed two or more like-named fields into the same struct, that isn't actually the case: it's fine to do that, as long as one of the fields has a smaller depth than the others, where the depth of a field is "the [minimum] number of embedded fields traversed to reach f".Trying to define this directly using recursion in QL is tricky, since (as far as the compiler can tell) it involves non-monotonic recursion through an aggregate. So instead we first collect a set of candidate fields with their depths, and then choose the shallowest. But when I wrote that code I didn't account for cyclic embeddings, which would make the predicate for collecting candidates infinite.
We now short-circuit by only looking through embedded fields if there isn't a field of the same name declared directly in the struct. I haven't been able to convince myself that this is sufficient, but it does fix the problem on
nomad, and I wasn't immediately able to construct an example where it still fails.